[SPARK-59014][SQL] Reject a data column that hides a captured metadata column during DSv2 refresh validation - #58295
Conversation
234bfb2 to
80cd41e
Compare
yyanyy
changed the title
…a column during DSv2 refresh validation `V2TableUtil.validateCapturedMetadataColumns` compared the captured metadata columns only against the metadata columns the connector still reports, so it could not see a conflict arriving on the data side. When a data column takes a captured metadata column's name and the connector suppresses the conflict (the default `canRenameConflictingMetadataColumns == false`), `metadataOutputWithOutConflicts` drops the metadata column: a captured reference to it becomes unresolvable, and on a partially-pruned scan `PushDownUtils.toOutputAttrs` collapses the two same-named fields so a query for the metadata column silently returns the data column's values. Detect that collision in the shared validator and report a user-facing error (`INCOMPATIBLE_TABLE_CHANGE_AFTER_ANALYSIS.METADATA_COLUMNS_MISMATCH`). Only the suppressed case is rejected; when the connector renames the conflicting metadata column it stays reachable and keeps working. The scan is skipped entirely when no captured metadata column survives the still-reported filter, so relations that project no metadata columns pay nothing. Tests cover the suppressed conflict in both validation modes, case-insensitive and case-sensitive matching, a renaming connector, and a metadata column the connector no longer reports. `InMemoryBaseTable` gains a `rename-conflicting-metadata-columns` table property, defaulting to true so existing suites are unaffected, which makes the suppressed branch reachable from a real query; `DataSourceV2DataFrameSuite` uses it to assert the error through the actual refresh path.
80cd41e to
3b163ad
Compare
…site Reverts the comment part of 8407d08. The existing one-line comment is already correct for the end state: once the companion validation lands, validation does own rejecting a suppressed metadata column. Annotating the transient window instead described a state that stops being true the moment apache#58295 lands, so it only created a follow-up edit to undo, and it changed no behavior in the meantime. The underlying question -- whether this PR is meant to land only after apache#58295, given that canRenameConflictingMetadataColumns defaults to false -- stays open for the author to answer rather than being recorded in code.
cloud-fan
left a comment
cloud-fan
left a comment
There was a problem hiding this comment.
Review summary
The refresh-validation design addresses the reported stale metadata-column failure, but its shadow check does not yet match Spark's resolver semantics. Please use the configured resolver and cover the Unicode case-folding counterexample; the nearby comment should also describe the later name-based reconciliation that actually produces the wrong value.
Findings
2 total: 0 P0, 1 P1, 0 P2, 1 P3.
Blocking (P1)
- Use the SQL resolver for shadow checks —
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/V2TableUtil.scala:160— see inline.
Nit (P3)
- Describe the actual stale-plan failure path —
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/V2TableUtil.scala:148— see inline.
| if (reportedMetaCols.isEmpty || renamesConflictingMetadataColumns(table)) { | ||
| Nil | ||
| } else { | ||
| val dataColNames = table.columns.iterator.map(c => normalize(c.name)).toSet |
There was a problem hiding this comment.
Blocking (P1): The planner decides metadata/data conflicts with the configured SQL resolver, but this check uses toLowerCase(Locale.ROOT). Those differ for valid Unicode identifiers: in the default case-insensitive mode, "\u0130ndex".equalsIgnoreCase("index") is true, while their root-locale lowercase strings are unequal (i\u0307ndex versus index). A captured metadata column can therefore pass this validation even though fresh resolution hides it, then reach name-based scan/output reconciliation and return the data column's values. Please compare the names with the same resolver used by the planner and add a Unicode regression test.
| * | ||
| * When a data column takes a metadata column's name, a connector that does not rename the | ||
| * conflict (`canRenameConflictingMetadataColumns` is false) suppresses the metadata column via | ||
| * `metadataOutputWithOutConflicts`. A suppressed metadata column can no longer be resolved, so a |
There was a problem hiding this comment.
Nit (P3): This explanation says the already-captured metadata attribute is suppressed and unresolvable, but metadataOutputWithOutConflicts returns metadata attributes already present in relation.output, and refresh preserves that output via r.copy(table = currentTable). The wrong-value path occurs later when PushDownUtils.toOutputAttrs reconciles the refreshed scan schema to retained output attributes by physical name, which can bind the same-named data field to the metadata attribute. Please reword this comment so it describes that mechanism.
cloud-fan
left a comment
cloud-fan
left a comment
There was a problem hiding this comment.
Review summary
The current head addresses the earlier resolver and Scaladoc feedback, and I found no remaining correctness, compatibility, documentation, or test-coverage issue that warrants a review finding.
Findings
0 total: 0 P0, 0 P1, 0 P2, 0 P3.
No findings.
Re-review status
2 addressed, 0 remaining, 0 new to this AI review.
New attribution: 0 newly introduced, 0 late catch, 0 previously raised, 0 unattributed.
Remaining findings
No prior AI findings remain.
2 participants
What changes were proposed in this pull request?
V2TableUtil.validateCapturedMetadataColumnsnow closes two related validation gaps:__metadata_col, rather than the possibly renamed physical attribute name.DSv2 relations capture their metadata-column attributes at analysis time and re-validate them when the table is refreshed / re-resolved. The existing check compared them only against the metadata columns the connector still reports, so a conflict arriving on the data side was invisible.
For connectors that rename conflicting metadata columns (
SupportsMetadataColumns.canRenameConflictingMetadataColumns()istrue), a metadata column logically namedindexcan appear in the relation output as_index. Previously, extraction looked up_indexin the connector's metadata columns, failed to match the logical nameindex, and silently skipped its type and nullability validation. The renamed column is now matched by its logical name. Such connectors remain unaffected by the shadowing rejection because the renamed metadata column remains reachable.For a fresh relation, when the conflict is suppressed instead (the default),
LogicalPlan.metadataOutputWithOutConflictsomits the metadata column. A refreshed relation, however, retains an already captured metadata attribute in its output. On a partially-pruned scan,PushDownUtils.toOutputAttrsthen reconciles the current scan schema to that retained output by physical name and can bind the same-named data field to the metadata attribute, so a query for the metadata column silently returns the data column's values.All three callers go through the new check, but only the two that admit new data columns can fire it: refresh (
ALLOW_NEW_FIELDS) and the dataframe temp-view path (ALLOW_NEW_TOP_LEVEL_FIELDS). UnderPROHIBIT_CHANGES,V2TableReference.validateNoChangesalready throws on the added data column first, so the transactional-write path is unchanged. Only metadata columns the table still reports are considered — one the connector dropped is already flagged as removed — and the check is skipped when none remain.The patch also corrects outdated comments:
__metadata_colstores the logical column name, while__file_source_metadata_colis the Boolean marker.Why are the changes needed?
Returning the wrong column's values is far worse than failing, and the
SupportsMetadataColumnscontract already recommends that non-renaming sources reject data columns that collide with metadata columns.Renaming a metadata attribute's physical name should also not bypass the existing schema compatibility validation.
Does this PR introduce any user-facing change?
Yes. A relation that referenced a metadata column and is later re-resolved against a table where a data column took that name now fails with "
<name>metadata column is hidden by a data column with the same name", instead of returning wrong results or working by luck depending on pruning. The condition isINCOMPATIBLE_TABLE_CHANGE_AFTER_ANALYSIS.METADATA_COLUMNS_MISMATCHon the refresh path andINCOMPATIBLE_COLUMN_CHANGES_AFTER_VIEW_WITH_PLAN_CREATION(colType=metadata) for a dataframe temp view.A renamed captured metadata column whose type or nullability later changes now also fails the existing schema compatibility validation instead of being silently skipped. Connectors that rename remain unaffected by the shadowing rejection when the metadata schema is unchanged. This tightens validation gaps on unreleased master.
How was this patch tested?
V2TableUtilSuitecovers the suppressed conflict under both validation modes, case sensitivity in both settings, the Unicode\u0130ndex/indexcase where SQL resolver semantics differ from root-locale lowercasing, a renamable connector, and a metadata column the connector stopped reporting (removed, not hidden).It also exercises the production
DataSourceV2Relation.withMetadataColumns()rename fromindexto_index, verifies that a subsequent type change is detected using the logical name, and verifies that an unchanged renamed metadata column remains valid.DataSourceV2DataFrameSuiteadds an end-to-end test where a newly added data column shadows a captured metadata column and the query now fails through the real refresh path. Reaching the suppressed branch needs a connector that does not rename, soInMemoryBaseTablegains arename-conflicting-metadata-columnsproperty defaulting totrue.catalyst/testOnly org.apache.spark.sql.connector.catalog.V2TableUtilSuite57 tests andsql/testOnly org.apache.spark.sql.connector.DataSourceV2DataFrameSuite -- -z "SPARK-59014"1 focused test, 0 failures. The Unicode test was also mutation-checked against the old root-locale comparison and failed as expected.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code 2.1.246